-
-
Notifications
You must be signed in to change notification settings - Fork 958
feat: mcp bundle tool integration #7595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c0836f7 to
512c885
Compare
512c885 to
a7b8b8f
Compare
d136520 to
b904a0d
Compare
rvanlaak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting that the mcp repo's schema PR is merged now, and you're able to spend time on API Platform integration.
Tried reviewing this PR without checking it out and testing it, so please feel free to ignore all my comments / suggestions / nitpicks 🙌 Just trying to provide you with another pov at some classes.
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As tell-dont-ask implies we expect creation of the operation, edge cases could lead to clear exceptions on how to resolve those. What about dropping nullability on the return typehint, and throw LogicException here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 and then remove the lines at:
https://github.com/api-platform/core/pull/7595/files/b904a0d0b94bc53bd5e701c7a1eb456ca255ae5c
| Put::class, | ||
| ApiResource::class, | ||
| ] as $class) { | ||
| $container->registerAttributeForAutoconfiguration($class, static function (ChildDefinition $definition): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about letting all these classes extend a Resource interface, and solely register the tag on that class? Or even better; consider the FQCN of that interface as the tag? Interface composition would be great way to document behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They all extend Metadata but I like this descriptive approach (not sure it works with a parent class also I'd need to check).
Not sure I understand the use of the fqcn tag as a namespace but we could easily add it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the #[AutoconfigureTag(Resource::class)] on such a "parent" Resource interface will register all implementation with FQCN as the tag on the container. Even the ones that extend a child interface. One of Symfony container's superpowers if you ask me.
If the functional goal for you would be to have all resource implementations available (probably somewhere later in a compiler pass?), then tagging them with the class name makes it possible to wire the entire collection of implementations with a single #[AutowireIterator(Resource::class)] attribute where needed.
class McpTool extends HttpOperation implements Resource { }
/**
* Extensive documentation about what Resource functionally is about....
*/
#[AutoconfigureTag(self::class)]
interface Resource {}
class ResourceCompilerPass {
public function __construct(
#[AutowireIterator(Resource::class)]
private iterable $resources
) { }
}To me, tagging with a FQCN is more descriptive as tagging with a simple textual tag, as it allows a single-click navigation to that interface in the IDE. The interface's documentation should then be descriptive enough to explain the rationales behind the interface segregation.
And, not having the foreach here also lowers maintenance efforts. There is no need for the entire foreach, and adding an extra type of resource then instantly makes them available in that potential compiler pass I'm referring to above as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the #[AutoconfigureTag(Resource::class)] on such a "parent" Resource interface will register all implementation with FQCN as the tag on the container. Even the ones that extend a child interface. One of Symfony container's superpowers if you ask me.
Yes but bundles can not use attributes for dependency injection but indeed this is a good point for DX.
👍 about the code you shared we should definitely do this and try to remove https://github.com/api-platform/core/blob/main/src/Metadata/Util/ReflectionClassRecursiveIterator.php. But this is another job that already started with #6943 btw
| use Symfony\Component\WebLink\Link as WebLink; | ||
|
|
||
| #[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)] | ||
| final class McpTool extends HttpOperation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the tool really the operation? Feels like there should be something in the middle of tool handling as well, to allow (like the symfony ai implementations) dispatch specific events around the handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand could you point me to some code or the component responsible of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feeling comes from Symfony Conference's "Symfony AI in Action" keynote that Christopher Hertel gave. https://live.symfony.com/account/replay/video/1128
Of course it eventually are the processors / providers that allow users to do what's needed, so I'm mostly referring to the lifecycle of such a MCP tool.
https://symfony.com/doc/current/ai/components/agent.html#tool-call-lifecycle-events
Hooking into the way how things are or are not propagated seems valuable, for instance with regards to continuing code execution or not? But, maybe the Symfony events referenced to above are sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agent tools (AsTool) and MCP (McpTool) are different things in the symfony/ai ecosystem, they don't interacts with Tool from MCP AFAIK (btw funny that they also use strings for tags, quite a convention for bundles: https://github.com/symfony/ai/blob/e885dbfab6115a89e6118fcc4d5dbe3567a72681/src/ai-bundle/src/AiBundle.php#L339)
Yeah it was the missing part + now I can also test out to use only the json-schema component's of API Platform to produce MCP SDK's schemas! |
Rework of #7458
Now with a simpler approach, this gives the ability to add McpTool (upcoming McpResource) to any API Platform environment giving access to:
item_uri_template)CallToolResult(from the php-sdk itself for images, sound etc.)TODO:
#[McpTool]attribute#[McpResource]Because MCP uses JSON-RPC under the hood with some particular response objects (from php-sdk) we can not change returned headers yet (we'll work on an extension point).